[envoy] Add support for HttpBody request by euroelessar · Pull Request #10110 · envoyproxy/envoy

euroelessar · 2020-02-19T22:28:27Z

Description: Add support for google.api.HttpBody message type in requests.
Support both unary and streaming requests.

Unary requests currently require filter to buffer the whole request before continue.
Streaming requests pass data to backend without any intermediate buffering at all.

Risk Level: Medium
Testing: Add unit and integration tests
Docs Changes: none
Release Notes: added
Fixes #10057

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

lizan

Thanks for working on this!

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h

test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc

lizan · 2020-02-20T01:24:52Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

+    }
+
+    bool done = !readToBuffer(*transcoder_->RequestOutput(), request_prefix_);
+    RELEASE_ASSERT(done, "request transcoder should have been done");


Just reject the request with error log? I don't think RELEASE_ASSERT should be done in data path of a filter.

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

lizan · 2020-02-20T01:31:05Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

  CONSTRUCT_ON_FIRST_USE(Http::LowerCaseString, "trailer");
 }

+constexpr uint32_t HttpBodyFieldNumber = 2;


inferer those from protobuf lib?

Can you advice if there is a presence of ProtobufLengthDelimitedField-like constant in protobuf, please? It looks like it's hardcoded as 2 everywhere in the libprotobuf codebase, and all potential helper functions are in internal namespace.

Yeah I see that is only in internal::WireFormat, can you move this constexpr to the function you use it (there's only one), and add a comment where it is from?

Done, added additional comment to already present link to wire format documentation.

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

lizan · 2020-02-20T21:39:34Z

I feel some part of this PR should go to https://github.com/grpc-ecosystem/grpc-httpjson-transcoding/ directly, thoughts? @qiwzhang

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

…quest-3 Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar · 2020-02-25T18:56:43Z

@lizan @qiwzhang Friendly ping

qiwzhang · 2020-02-27T03:30:34Z

@lizan I can see the benefit of moving some of code to grpc_httpjson_transcoder repo: it will enhance the library to support HttpBody type for both request and response and other callers will benefit from such enhancement.

But I also understand that HttpBody translation is different with JSON translation. It seems that it is not easy to fit HttpBody translation code into that library. I am fine to have specific HttpBody translation code here.

qiwzhang · 2020-02-27T19:50:20Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

+      return status;
+    }
+
+    if (method_info->request_body_field_path.empty()) {


do we handle case where the request type is HttpBody and http_rule.body() is *?

if so, should the ending block at line 248 move to line 237?

From reading google cloud's transcoding documentation it looks like "*" body is only applicable to json transcoding:

The special name * can be used in the body mapping to define that every field not bound by the path template should be mapped to the request body.

https://cloud.google.com/endpoints/docs/grpc-service-config/reference/rpc/google.api#google.api.HttpRule

Then, do we support the HttpBody as the gRPC request type?
with auto_mapping =true, we generate a http rule with * body for every grpc method.

Do you happen to know what is a behavior of Google Cloud in this case?
I'm probably inclined to treat "*" as "" for HttpBody requests.

qiwzhang · 2020-02-27T20:34:33Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

  return false;
 }

+void JsonTranscoderFilter::createHttpBodyEnvelope(Buffer::Instance& data, uint64_t content_length) {


I feel this code is grpc specific, can be moved to grpc util library. it should be unit-tested thoroughly.

Few concerns:

It's more protobuf than grpc specific (grpc piece is small and is not a concern)

I'm not sure it's useful anywhere outside of this filter

I agree though that it's not trivial and I'm adding better test coverage

Few concerns:

It's more protobuf than grpc specific (grpc piece is small and is not a concern)

I'm not sure it's useful anywhere outside of this filter

I agree though that it's not trivial and I'm adding better test coverage

This is both grpc and protobuf specific because of the grpc framing part. Ideally it should be in the transcoding library, and a fuzz test should be added too.

gRPC-specific code was moved to shared library so it's re-used with http1 reverse bridge.
HttpBody protobuf-specific code was covered by tests for the edge cases.

I'm not sure what would be a benefit of a fuzz testing in this case (user input is limited and well-defined).

qiwzhang · 2020-02-27T20:35:39Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

+  // Embedded messages are treated the same way as strings (wire type 2).
+  constexpr uint32_t ProtobufLengthDelimitedField = 2;
+
+  std::string request_prefix = request_prefix_.toString();


Not quite understand what request_prefix is for? could not add some examples in the comment

…quest-3 Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

stale · 2020-03-12T06:02:49Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

lizan · 2020-03-12T22:55:13Z

not stable, I'm back from OOO.

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

…quest-3 Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

lizan · 2020-03-18T08:33:28Z

code LGTM, can you fix CI?

…quest-3 Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar · 2020-03-18T10:22:03Z

ci/circleci: go_control_plane_mirror looks like a flake (failure to download http archive from github.com)

Ruslan Nigmatullin added 2 commits February 19, 2020 14:10

[envoy] Add support for HttpBody request

e08b9a1

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

release note

30822a9

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar requested a review from lizan as a code owner February 19, 2020 22:28

lizan self-assigned this Feb 19, 2020

lizan reviewed Feb 20, 2020

View reviewed changes

Ruslan Nigmatullin added 3 commits February 20, 2020 10:27

cr

54910f1

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

more descriptive debug log

2388569

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

use TypeSharedPtr, fix username in TODO

5a36d37

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

lizan requested a review from qiwzhang February 20, 2020 21:39

Ruslan Nigmatullin added 3 commits February 20, 2020 13:44

move const inside, add comment

ea66541

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

sed in tests as well

48cea14

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Merge branch 'master' of github.com:envoyproxy/envoy into httpbody-re…

409935d

…quest-3 Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

dounan mentioned this pull request Feb 26, 2020

Support google.api.HttpBody for requests in gRPC-JSON transcoder #7007

Closed

qiwzhang reviewed Feb 27, 2020

View reviewed changes

Merge branch 'master' of github.com:envoyproxy/envoy into httpbody-re…

eaeaab7

…quest-3 Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

stale bot added stale stalebot believes this issue/PR has not been touched recently and removed stale stalebot believes this issue/PR has not been touched recently labels Mar 12, 2020

Ruslan Nigmatullin added 3 commits March 17, 2020 11:56

share code with reverse bridge & restructure for testability

bde5440

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Merge branch 'master' of github.com:envoyproxy/envoy into httpbody-re…

827ccd4

…quest-3 Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

add test for http body encoding

514a440

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar requested review from snowp and zuercher as code owners March 17, 2020 21:37

cleanup and more tests

ae4b6fc

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

format

a06bcc7

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Merge branch 'master' of github.com:envoyproxy/envoy into httpbody-re…

ed139b4

…quest-3 Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

qiwzhang approved these changes Mar 18, 2020

View reviewed changes

lizan approved these changes Mar 19, 2020

View reviewed changes

lizan merged commit 4283e25 into envoyproxy:master Mar 19, 2020

Gurpartap mentioned this pull request May 2, 2021

How to read input http body bytes using gRPC-JSON Transcoder? #7411

Closed

ghevge mentioned this pull request May 3, 2021

Handling Binary File Uploads grpc-ecosystem/grpc-gateway#500

Open

rmilejcz mentioned this pull request Apr 11, 2022

Question: How to properly use google.api.httpBody with gRPC-JSON transcoder to accept arbitrary content? #20772

Closed

vkarunx mentioned this pull request Apr 26, 2022

fn command-history v1 namespacelabs/foundation#289

Merged

vkarunx mentioned this pull request Jun 14, 2022

Resilient handling when envoy fails to build a proto descriptor pool namespacelabs/foundation#604

Closed

timburks mentioned this pull request Sep 9, 2022

Upload file HTTP-GRPC transcoding GoogleCloudPlatform/esp-v2#666

Closed

Conversation

euroelessar commented Feb 19, 2020

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan commented Feb 20, 2020

Uh oh!

euroelessar commented Feb 25, 2020

Uh oh!

qiwzhang commented Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qiwzhang Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Mar 12, 2020

Uh oh!

lizan commented Mar 12, 2020

Uh oh!

lizan commented Mar 18, 2020

Uh oh!

euroelessar commented Mar 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

qiwzhang commented Feb 27, 2020 •

edited

Loading

qiwzhang Feb 27, 2020 •

edited

Loading